Test/ run-opts-configurable-host by intojhanurag · Pull Request #3293 · knative/func

intojhanurag · 2025-12-11T20:18:35Z

supersedes : #3275

codecov · 2025-12-11T20:36:09Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 54.71%. Comparing base (be256cd) to head (f0f35b7).
⚠️ Report is 28 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3293      +/-   ##
==========================================
- Coverage   54.74%   54.71%   -0.03%     
==========================================
  Files         168      168              
  Lines       19605    19604       -1     
==========================================
- Hits        10732    10726       -6     
- Misses       7807     7813       +6     
+ Partials     1066     1065       -1

Flag	Coverage Δ
e2e	`40.32% <33.33%> (+<0.01%)`	⬆️
e2e go	`36.28% <33.33%> (-0.01%)`	⬇️
e2e node	`31.74% <0.00%> (+0.01%)`	⬆️
e2e python	`35.93% <33.33%> (-0.09%)`	⬇️
e2e quarkus	`31.89% <0.00%> (+0.01%)`	⬆️
e2e rust	`31.35% <0.00%> (+0.01%)`	⬆️
e2e springboot	`31.37% <0.00%> (+0.03%)`	⬆️
e2e typescript	`31.86% <0.00%> (+0.01%)`	⬆️
integration	`17.74% <0.00%> (+0.03%)`	⬆️
unit macos-14	`44.42% <100.00%> (+0.01%)`	⬆️
unit macos-latest	`44.42% <100.00%> (+0.01%)`	⬆️
unit ubuntu-24.04-arm	`44.62% <100.00%> (+0.01%)`	⬆️
unit ubuntu-latest	`45.34% <100.00%> (+0.01%)`	⬆️
unit windows-latest	`44.44% <100.00%> (+0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

intojhanurag · 2025-12-12T05:04:00Z

/ok-to-test

intojhanurag · 2025-12-12T05:05:27Z

Hey @gauron99 , Please take a look on it . I added the suggestions of the preivous PR ( because that PR was messed up ).

gauron99

Thanks! just a few naming nits. See below

gauron99 · 2025-12-12T11:53:55Z

pkg/functions/runner.go

+	if val == "" {
+		val = "localhost"
+	}
+	return val, "8080"


use the default values defined as variables for host and port. Also the TODO at the top of the file // TODO allow to be altered via a runOpt can be removed since we are adding this option to edit the host

pkg/functions/runner_test.go

pkg/functions/runner.go

pkg/functions/runner_test.go

intojhanurag · 2025-12-12T16:59:57Z

Hey @gauron99 , Now is it good to go ?

gauron99

Thanks for the cleanups. I reviewed the functionality and have some suggestions. I tested the functionality a little locally and I think my suggestions should cover all the cases. If you discover something Im missing please let me know!

gauron99 · 2025-12-13T15:01:37Z

pkg/functions/runner_test.go

 	}
 }
+
+func TestParseAddress(t *testing.T) {


every case which has implicit defaulting should expect the defaultX variable value instead of hardcoded string so the test doesnt have to be re-written if the default changes

pkg/functions/runner.go

intojhanurag · 2025-12-13T17:36:03Z

Hey @gauron99, PTAL now , Good eye btw :)

lkingland · 2025-12-15T05:33:54Z

pkg/functions/runner.go

 	err    io.Writer
 }

+func ParseAddress(val string) (host, port string, explicitPort bool) {


Since this is a "utility function" used only here, there's no need for it to be exported (can be just parseAddress)

Hey @lkingland , but this function has been used in test file , so i made this ParseAddress

lkingland · 2025-12-15T06:09:36Z

pkg/functions/runner.go

 )

 const (
-	defaultRunHost    = "127.0.0.1" // TODO allow to be altered via a runOpt


I believe this is still an outstanding TODO

Arent we allowing the possibility to alter host with this flag? Or was the TODO for something more specific?

Hey @lkingland , but now if a user will give his own Host then we have a function to adapt that host , so indirectly this todo has been solved .btw what do you think here ?

lkingland · 2025-12-15T06:14:57Z

@intojhanurag I am a bit confused on this PR. It's labeled as a follow-up to #3275 but currently contains only a slight refactor. Is this intended to be a draft?

intojhanurag · 2025-12-15T06:20:12Z

@intojhanurag I am a bit confused on this PR. It's labeled as a follow-up to #3275 but currently contains only a slight refactor. Is this intended to be a draft?

No i think , i used a wrong word "follow up" , Actually in that PR initially i used totally differenct logic then Matej suggested diff idea , that i applied in that PR at that time . But again i thought to open a new PR 😭 because there was some issue locally regarding merge conflicts.

knative-prow · 2025-12-15T06:28:53Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: intojhanurag
Once this PR has been reviewed and has the lgtm label, please ask for approval from lkingland. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

lkingland · 2025-12-15T07:08:21Z

The parseAddress() function looks fine, but it won't be reachable by users because cmd/run.go (lines 419-438) validates the address using net.SplitHostPort() and rejects anything without both host and port before it ever reaches the new function here. Correct me if I am wrong, but I think we may need to update validateRunConfig() in cmd/run.go to remove the strict host+port requirement. Once that's done, users will actually be able to run func run --address 0.0.0.0 as I believe is intended.

intojhanurag · 2025-12-15T12:28:12Z

The parseAddress() function looks fine, but it won't be reachable by users because cmd/run.go (lines 419-438) validates the address using net.SplitHostPort() and rejects anything without both host and port before it ever reaches the new function here. Correct me if I am wrong, but I think we may need to update validateRunConfig() in cmd/run.go to remove the strict host+port requirement. Once that's done, users will actually be able to run func run --address 0.0.0.0 as I believe is intended.

@lkingland , Yeah you are right . I tried locally , it is not working accordingly. what i should do now ?
I think we should only remove todo , and should not make this complicate , if user will enter only host then he will get warn for PORT.

Do refactoring and add tests

62856a1

knative-prow bot requested review from dsimansk and jrangelramos December 11, 2025 20:18

knative-prow bot added the size/L 🤖 PR changes 100-499 lines, ignoring generated files. label Dec 11, 2025

intojhanurag added 2 commits December 11, 2025 20:22

linting issue

579d23e

remove comment

992d073

knative-prow bot added size/M 🤖 PR changes 30-99 lines, ignoring generated files. and removed size/L 🤖 PR changes 100-499 lines, ignoring generated files. labels Dec 11, 2025

knative-prow bot added the ok-to-test 🤖 Non-member PR verified by an org member that is safe to test. label Dec 12, 2025

gauron99 reviewed Dec 12, 2025

View reviewed changes

code rafactoring and naming conventions

4374130

knative-prow bot added size/L 🤖 PR changes 100-499 lines, ignoring generated files. and removed size/M 🤖 PR changes 30-99 lines, ignoring generated files. labels Dec 12, 2025

linting issue

3dd618b

gauron99 reviewed Dec 13, 2025

View reviewed changes

intojhanurag added 2 commits December 13, 2025 17:16

Added edgecases

ce7ebd6

linting issue

f0f35b7

lkingland requested changes Dec 15, 2025

View reviewed changes

knative-prow bot assigned lkingland Dec 15, 2025

intojhanurag force-pushed the Test/run-opts-configurable-host branch from fc9cbfb to f0f35b7 Compare December 15, 2025 06:31

intojhanurag closed this Jan 6, 2026

Conversation

intojhanurag commented Dec 11, 2025 • edited by matejvasek Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Dec 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

intojhanurag commented Dec 12, 2025

Uh oh!

intojhanurag commented Dec 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gauron99 left a comment

Choose a reason for hiding this comment

Uh oh!

gauron99 Dec 12, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

intojhanurag commented Dec 12, 2025

Uh oh!

gauron99 left a comment

Choose a reason for hiding this comment

Uh oh!

gauron99 Dec 13, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

intojhanurag commented Dec 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lkingland Dec 15, 2025

Choose a reason for hiding this comment

Uh oh!

intojhanurag Dec 15, 2025

Choose a reason for hiding this comment

Uh oh!

lkingland Dec 15, 2025

Choose a reason for hiding this comment

Uh oh!

gauron99 Dec 15, 2025

Choose a reason for hiding this comment

Uh oh!

intojhanurag Dec 15, 2025

Choose a reason for hiding this comment

Uh oh!

lkingland commented Dec 15, 2025

Uh oh!

intojhanurag commented Dec 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

knative-prow bot commented Dec 15, 2025

Uh oh!

lkingland commented Dec 15, 2025

Uh oh!

intojhanurag commented Dec 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

intojhanurag commented Dec 11, 2025 •

edited by matejvasek

Loading

codecov bot commented Dec 11, 2025 •

edited

Loading

intojhanurag commented Dec 12, 2025 •

edited

Loading

intojhanurag commented Dec 13, 2025 •

edited

Loading

intojhanurag commented Dec 15, 2025 •

edited

Loading

intojhanurag commented Dec 15, 2025 •

edited

Loading